Skip to content

Use page associations to get routes#2273

Merged
stephencdaly merged 4 commits into
mainfrom
use-page-associations-to-get-routes
Oct 9, 2025
Merged

Use page associations to get routes#2273
stephencdaly merged 4 commits into
mainfrom
use-page-associations-to-get-routes

Conversation

@stephencdaly

Copy link
Copy Markdown
Contributor

What problem does this pull request solve?

Previously conditions were retrieved from forms-api and returned as part of the Form response. Now they are stored in the forms-admin database, so we can use ActiveRecord associations to get them directly for a given page and reduce the number of SQL queries executed.

This means we can remove PageConditionsService and PageRoutesService.

Things to consider when reviewing

  • Ensure that you consider the wider context.
  • Does it work when run on your machine?
  • Is it clear what the code is doing?
  • Do the commit messages explain why the changes were made?
  • Are there all the unit tests needed?
  • Do the end to end tests need updating before these changes will pass?
  • Has all relevant documentation been updated?

@stephencdaly stephencdaly force-pushed the use-page-associations-to-get-routes branch 4 times, most recently from 6765a8d to 2c3ba8b Compare October 8, 2025 16:48
Comment thread app/models/page.rb Outdated
@stephencdaly stephencdaly force-pushed the use-page-associations-to-get-routes branch from 2c3ba8b to bae0b43 Compare October 9, 2025 10:29

@lfdebrux lfdebrux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay for fewer services! Thanks for doing this

Previously conditions were retrieved from forms-api and returned as
part of the Form response. Now they are stored in the forms-admin
database, so we can use ActiveRecord associations to get them directly
for a given page and reduce the number of SQL queries executed.
Add a method to get the secondary skip condition for a page if one
exists and use this in the places we were previously using
PageRoutesService to simplify how we retrieve the condition.
PageRoutesService and PageConditionsService are no longer used so can
be deleted.
Previously, we had to reload the pages after adding conditions as we
were mocking out the method for retrieving them. Now that conditions
are retreived from the database, these reloads are unnecessary in
request specs as the code under test will retrieve the latest data
from the database.
@stephencdaly stephencdaly force-pushed the use-page-associations-to-get-routes branch from bae0b43 to 2de90a5 Compare October 9, 2025 10:44
@sonarqubecloud

sonarqubecloud Bot commented Oct 9, 2025

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Oct 9, 2025

Copy link
Copy Markdown

🎉 A review copy of this PR has been deployed! You can reach it at: https://pr-2273.admin.review.forms.service.gov.uk/

It may take 5 minutes or so for the application to be fully deployed and working. If it still isn't ready
after 5 minutes, there may be something wrong with the ECS task. You will need to go to the integration AWS account
to debug, or otherwise ask an infrastructure person.

For the sign in details and more information, see the review apps wiki page.

@stephencdaly stephencdaly merged commit c13ffbe into main Oct 9, 2025
6 checks passed
@stephencdaly stephencdaly deleted the use-page-associations-to-get-routes branch October 9, 2025 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants